-
Notifications
You must be signed in to change notification settings - Fork 434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: adds tutor config edit
#1099
base: nightly
Are you sure you want to change the base?
feat: adds tutor config edit
#1099
Conversation
tutor/commands/config.py
Outdated
@click.command(name="edit", help="Edit config.yml of the current environment") | ||
@click.pass_obj | ||
def edit(context: Context) -> None: | ||
config_file = os.path.join(context.root, "config.yml") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use config.config_path(context.root)
instead.
tutor/commands/config.py
Outdated
elif which("start"): # Windows | ||
open_cmd = ["start", '""', config_file] | ||
else: | ||
click.echo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command should fail if neither open commands are available. You could for instance raise exceptions.TutorError
.
tutor/commands/config.py
Outdated
|
||
open_cmd = None | ||
|
||
if which("open"): # MacOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's amend the comment. open
is also available on Ubuntu, for instance.
I have another, more important problem with this command. According to my testing, when I run open file.txt
on a headless server (for instance: a remote production server), it opens a read-only editor. How can we fix this command to open a write-capable editor? (xdg-open
is not available on servers without a Desktop) Note that $EDITOR
is set to "vim" on this server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@regisb I wanted to test this scenario to evaluate the options. However, the server image I used (ubuntu based) didn't have a open
at all. And my desktop with Ubuntu seems to just ship xdg-open
as open
.
❯ which open
/usr/bin/open
❯ open --version
xdg-open 1.1.3
when I run
open file.txt
on a headless server (for instance: a remote production server), it opens a read-only editor.
Which version of open
is this, and what's the editor that it opens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the topic of a "writable editor", MacOS's open
implements -a
flag, which allows specifying the application to open the file. I am wondering if something similar can be added to this command, that can force a specific editor to be launched. tutor config edit -a vim
maybe? What do think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be a good option, this gives control to the user. For instance, if someone wants to open the file in Sublime/VScode, they can override the default utils the command is checking.
tutor/commands/config.py
Outdated
open_cmd = ["start", '""', config_file] | ||
else: | ||
click.echo( | ||
"Cannot find a way to open the editor automatically. Kindly open the file manually: " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rephrase that along the lines of: "Failed to detect an adequate text file editor". (mode concise, no need for "kindly")
) | ||
return | ||
|
||
subprocess.call(open_cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command will do something funky if the file or its directory does not exist. I suggest the command should fail if the config file does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! The open
s I tested all launched the browser with the argument as the URL.
@regisb Hi, Thanks for the review comments. I hoped to have addressed them by now. Somehow haven't found the time. I will handle the comments this week. |
I have nothing to add beyond Regis's review--just want to say that this is a great idea and thank you for the contribution 👍🏻 |
ecf0b53
to
853e51e
Compare
elif which("start"): # Windows | ||
open_cmd = ["start", '""', config_file] | ||
else: | ||
raise exceptions.TutorError(f"Failed to find utility to launch an editor.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the error message, we can specify which utilities are used by the command internally so that user can install them on the system if needed (not all the users of Tutor are dev, some are operators who might not want to delve into code to see what is happening under the hood).
Quickly launch the default YAML editor for editing the config.yml file.
853e51e
to
c095e45
Compare
Hi @tecoholic . I'm looking forward to this change! Looks like there are just a couple minor change requests left, and then we're good to merge this. |
@kdmccormick Hi, thanks for the ping. I was waiting on @regisb for more details about read only editor. Can we skip that scenario for now? |
@kdmccormick I am less interested in this feature than you seem to be -- mostly because (IMHO) it doesn't bring anything more than a .bashrc alias. E.g: |
Quickly launch the default YAML editor for editing the
config.yml
file.Background
As a developer working with multiple projects, I am constantly changing the tutor config and re-running the
tutor dev launch
to update the devstack. While it is as simple as runningvim $(tutor config printroot)/config.yml
, I often found myself wanting to simply express this better using tutor config edit. So this is an attempt at cross-platform solution doing the same. It might not be the best solution, but something to start with if someone else finds this useful and improves their DevEx.Caveat: I have only tested this on Linux (both open and
xdg-open
works in mine, so it could be said Unix).